Bound duplicated text-heavy tool output#85
Conversation
📝 WalkthroughWalkthroughAdds configurable inline output limits, a preview/metadata utility, and bounded response shaping for shell, read, grep, glob, and ls tools. CLI configuration handling, documentation, tests, and widget-aware metadata are updated accordingly. ChangesInline output previews
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/tool-output.ts (1)
64-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUntested fallback branch.
The
maxCharacters <= markerCharactershead-only fallback (reachable with a small but valid configured limit) isn't exercised intool-output.test.ts. Worth a regression test to lock in this edge case, since a small user-configuredinlineOutputCharacterswould hit it silently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tool-output.ts` around lines 64 - 74, Add a regression test in tool-output.test.ts covering the maxCharacters <= markerCharacters fallback in the tool-output formatting flow. Configure a small valid inlineOutputCharacters limit that triggers the head-only path, then assert the returned text, character counts, omittedCharacters, and truncated flag match the expected behavior.src/server.ts (1)
1028-1058: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated bounded-response assembly across 5 tool handlers.
read(here),grep(1400-1429),glob(1470-1499),ls(1540-1565), andshell(1626-1657) each hand-roll the same sequence: bound error content viaboundResponseContent, then on success buildbounded, mergeboundedSummaryinto a tool-specific summary, calltoolResultMeta, and setstructuredContentviaboundedStructuredContent.processToolResponse(lines 581-609) already shows the pattern of extracting this into one shared helper forexec_command/write_stdin— applying the same treatment here would remove ~5x near-duplicated blocks and centralize future changes to the bounding contract.A shared helper could take
(config, kind, tool, workspaceId, path, response, extraSummaryFields)and return the final{ content, _meta, structuredContent }shape, with each handler only supplying its tool-specific summary fields (pattern/scope, offset/limited, command/workingDirectory, etc.).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server.ts` around lines 1028 - 1058, Extract the repeated bounded-response assembly from the read, grep, glob, ls, and shell handlers into a shared helper, following the existing processToolResponse pattern. Have the helper accept the config, tool kind/name, workspace and path context, response, and tool-specific summary fields, then centralize boundResponseContent, boundedSummary, toolResultMeta, and boundedStructuredContent while preserving each handler’s existing metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tool-output.ts`:
- Around line 15-93: Optimize createOutputPreview by avoiding repeated
full-string code-point materialization: first use value.length as a safe fast
path when the string is within maxCharacters, otherwise create one reusable
code-point array and use it for the original character count and head/tail
slicing. Update takeHead and takeTail or replace their use within
createOutputPreview so they consume the shared array, while preserving
lineCount, truncation boundaries, and reported counts.
---
Nitpick comments:
In `@src/server.ts`:
- Around line 1028-1058: Extract the repeated bounded-response assembly from the
read, grep, glob, ls, and shell handlers into a shared helper, following the
existing processToolResponse pattern. Have the helper accept the config, tool
kind/name, workspace and path context, response, and tool-specific summary
fields, then centralize boundResponseContent, boundedSummary, toolResultMeta,
and boundedStructuredContent while preserving each handler’s existing metadata.
In `@src/tool-output.ts`:
- Around line 64-74: Add a regression test in tool-output.test.ts covering the
maxCharacters <= markerCharacters fallback in the tool-output formatting flow.
Configure a small valid inlineOutputCharacters limit that triggers the head-only
path, then assert the returned text, character counts, omittedCharacters, and
truncated flag match the expected behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 84b23e28-2318-4415-8b94-1ffa2549c6e9
📒 Files selected for processing (9)
docs/configuration.mdpackage.jsonsrc/cli.tssrc/config.test.tssrc/config.tssrc/server.tssrc/tool-output.test.tssrc/tool-output.tssrc/user-config.ts
| function codePoints(value: string): string[] { | ||
| return Array.from(value); | ||
| } | ||
|
|
||
| function characterLength(value: string): number { | ||
| return codePoints(value).length; | ||
| } | ||
|
|
||
| function lineCount(value: string): number { | ||
| if (value.length === 0) return 0; | ||
| return value.endsWith("\n") | ||
| ? value.slice(0, -1).split("\n").length | ||
| : value.split("\n").length; | ||
| } | ||
|
|
||
| function takeHead(value: string, count: number): string { | ||
| if (count <= 0) return ""; | ||
| return codePoints(value).slice(0, count).join(""); | ||
| } | ||
|
|
||
| function takeTail(value: string, count: number): string { | ||
| if (count <= 0) return ""; | ||
| const characters = codePoints(value); | ||
| return characters.slice(Math.max(0, characters.length - count)).join(""); | ||
| } | ||
|
|
||
| export function createOutputPreview( | ||
| value: string, | ||
| maxCharacters = DEFAULT_INLINE_OUTPUT_CHARACTERS, | ||
| ): OutputPreview { | ||
| if (!Number.isInteger(maxCharacters) || maxCharacters < 1) { | ||
| throw new Error("Inline output limit must be a positive integer."); | ||
| } | ||
|
|
||
| const originalCharacters = characterLength(value); | ||
| const originalLines = lineCount(value); | ||
|
|
||
| if (originalCharacters <= maxCharacters) { | ||
| return { | ||
| text: value, | ||
| originalCharacters, | ||
| originalLines, | ||
| inlineCharacters: originalCharacters, | ||
| omittedCharacters: 0, | ||
| truncated: false, | ||
| }; | ||
| } | ||
|
|
||
| const markerCharacters = characterLength(TRUNCATION_MARKER); | ||
| if (maxCharacters <= markerCharacters) { | ||
| const text = takeHead(value, maxCharacters); | ||
| return { | ||
| text, | ||
| originalCharacters, | ||
| originalLines, | ||
| inlineCharacters: characterLength(text), | ||
| omittedCharacters: originalCharacters - characterLength(text), | ||
| truncated: true, | ||
| }; | ||
| } | ||
|
|
||
| const available = maxCharacters - markerCharacters; | ||
| const headCharacters = Math.ceil(available * 0.65); | ||
| const tailCharacters = available - headCharacters; | ||
| const text = | ||
| takeHead(value, headCharacters) + | ||
| TRUNCATION_MARKER + | ||
| takeTail(value, tailCharacters); | ||
| const inlineCharacters = characterLength(text); | ||
|
|
||
| return { | ||
| text, | ||
| originalCharacters, | ||
| originalLines, | ||
| inlineCharacters, | ||
| omittedCharacters: originalCharacters - headCharacters - tailCharacters, | ||
| truncated: true, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Multiple full-string passes on the truncation hot path.
For large value (the exact case this feature targets — big file reads, verbose grep/ls/shell output), createOutputPreview performs several independent O(n) full-string materializations: characterLength(value) (line 49), lineCount(value)'s .split("\n") (line 50), then takeHead(value, ...) and takeTail(value, ...) each call codePoints(value) again (lines 65/80-82). Each of these builds a brand-new array holding every code point of the entire original string, even though only a ~12,000-character preview is ultimately needed. This scales poorly with input size and works against the PR's own goal of bounding cost for text-heavy tools.
A low-risk improvement: compute the code-point array once and reuse it for length, head, and tail instead of re-deriving it 3+ times; additionally, a fast path using value.length (UTF‑16 length is always ≥ code-point length) can skip the expensive path entirely whenever the string is already within bounds.
♻️ Suggested optimization sketch
function createOutputPreview(
value: string,
maxCharacters = DEFAULT_INLINE_OUTPUT_CHARACTERS,
): OutputPreview {
if (!Number.isInteger(maxCharacters) || maxCharacters < 1) {
throw new Error("Inline output limit must be a positive integer.");
}
- const originalCharacters = characterLength(value);
- const originalLines = lineCount(value);
-
- if (originalCharacters <= maxCharacters) {
+ const originalLines = lineCount(value);
+
+ // Fast path: UTF-16 length is always >= code-point length, so if it already
+ // fits, we know the code-point count fits too without materializing the array.
+ if (value.length <= maxCharacters) {
+ const originalCharacters = characterLength(value);
+ if (originalCharacters <= maxCharacters) {
return {
text: value,
originalCharacters,
originalLines,
inlineCharacters: originalCharacters,
omittedCharacters: 0,
truncated: false,
};
+ }
}
+ const points = codePoints(value); // single materialization, reused below
+ const originalCharacters = points.length;
const markerCharacters = characterLength(TRUNCATION_MARKER);
...
- const text = takeHead(value, headCharacters) + TRUNCATION_MARKER + takeTail(value, tailCharacters);
+ const text =
+ points.slice(0, headCharacters).join("") +
+ TRUNCATION_MARKER +
+ points.slice(Math.max(0, points.length - tailCharacters)).join("");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tool-output.ts` around lines 15 - 93, Optimize createOutputPreview by
avoiding repeated full-string code-point materialization: first use value.length
as a safe fast path when the string is within maxCharacters, otherwise create
one reusable code-point array and use it for the original character count and
head/tail slicing. Update takeHead and takeTail or replace their use within
createOutputPreview so they consume the shared array, while preserving
lineCount, truncation boundaries, and reported counts.
|
I apologize, my agent got a little trigger happy and sent out a PR without my approval, I told him to fork it not send you a PR. I will continue to work on this but I wasn't planning on sending out a PR until I reached a solid implementation of the issue I was running into. You can ignore or review, up to you. |
lol, these days agents are going vague, was it gpt 5.6 sol? |
Lmao yeah it was. I mentioned early in the session that if we find a solution we could possibly send out a PR and it just sent it out on our first iteration 😂 My fault, gotta think about how to sandbox devspace without requiring approval for everything. |
|
btw do web research about which content gets exposed to model from MCP hosts, I implemented it depending on that so as far as I remember chatgpt does not give both to model so it is safe to assume context doesn't get polluted by repeating things but yeah they recently did some MCP UI overhaul changes, not sure, do verify it by asking model what tool call output visible to ya or some question similar to it |
2 participants
Problem
Text-heavy tools copied the same output into content, structuredContent.result, and the widget card payload. Large shell and process responses therefore amplified one result across model-visible transcript and UI channels.
Fix
Regression coverage
A deterministic 50000-character response test confirms the previous three-channel payload exceeds 150 KB while the bounded full-widget response stays below 30 KB and below one quarter of the original size.
Validation
Summary by CodeRabbit